Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block supports: refactor how the block to render is tracked to improve core/plugin compatibility #26356

Merged
merged 10 commits into from
Oct 22, 2020

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Oct 21, 2020

See WordPress/wordpress-develop#640

This PR removes the global $current_block_parsed (that was used to track which block is in to be rendered) in favor of making it a static property of the WP_Block_Supports class.

@github-actions
Copy link

github-actions bot commented Oct 21, 2020

Size Change: 0 B

Total Size: 1.2 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.54 kB 0 B
build/api-fetch/index.js 3.35 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 668 B 0 B
build/block-directory/index.js 8.59 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 130 kB 0 B
build/block-editor/style-rtl.css 11 kB 0 B
build/block-editor/style.css 11 kB 0 B
build/block-library/editor-rtl.css 8.93 kB 0 B
build/block-library/editor.css 8.93 kB 0 B
build/block-library/index.js 145 kB 0 B
build/block-library/style-rtl.css 7.75 kB 0 B
build/block-library/style.css 7.75 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.77 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.6 kB 0 B
build/components/index.js 170 kB 0 B
build/components/style-rtl.css 15.4 kB 0 B
build/components/style.css 15.4 kB 0 B
build/compose/index.js 9.63 kB 0 B
build/core-data/index.js 12.1 kB 0 B
build/data-controls/index.js 684 B 0 B
build/data/index.js 8.63 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 4.43 kB 0 B
build/edit-navigation/index.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.37 kB 0 B
build/edit-post/style.css 6.35 kB 0 B
build/edit-site/index.js 21.7 kB 0 B
build/edit-site/style-rtl.css 3.79 kB 0 B
build/edit-site/style.css 3.79 kB 0 B
build/edit-widgets/index.js 26.6 kB 0 B
build/edit-widgets/style-rtl.css 3.09 kB 0 B
build/edit-widgets/style.css 3.09 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 42.6 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.49 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 709 B 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/index.js 3.02 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.45 kB 0 B
build/primitives/index.js 1.35 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13 kB 0 B
build/server-side-render/index.js 2.61 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.75 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.23 kB 0 B

compressed-size-action

@ZebulanStanphill ZebulanStanphill added the [Type] Code Quality Issues or PRs that relate to code quality label Oct 21, 2020
@youknowriad
Copy link
Contributor

Looks good to me, wdyt @mcsf

add_action( 'init', array( 'WP_Block_Supports', 'init' ), 22 );
add_filter( 'register_block_type_args', 'wp_block_supports_track_block_to_render' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that filter is better where it was in compat.php because it's specifically about that right? compatibility with WP versions that don't support that variable yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup:

  • if the plugin is active on a WP version that doesn't have this class => this will be loaded.
  • if the plugin is active on a WP version that does have the class => this won't be loaded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, but do we need to signal anywhere that this class shouldn't be loaded when the plugin requires WP 5.6? I'm not sure how that works.

Copy link
Member Author

@oandregal oandregal Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing we could do would be to move this code from load.php:

if ( ! class_exists( 'WP_Block_Supports' ) ) {
	require_once dirname( __FILE__ ) . '/class-wp-block-supports.php';
}

to compat.php (and add the comment about WP 5.6).

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the goals is to only set up the register_block_type_args filter when the WP_Block_Supports class loaded in the runtime is the one from the plugin and not core, correct? If so, what if the plugin's version of the class provided its own wp_block_supports_track_block_to_render as a static method? That way, we would have:

  • Conditional class loading in load.php, as is the case for all other classes
  • In compat.php, something like:
if ( class_exists( 'WP_Block_Supports' ) && method_exists( 'WP_Block_Supports', 'track_block_to_render' ) ) {
    add_filter( 'register_block_type_args', array( 'WP_Block_Support', 'track_block_to_render' ) );
}

Or am I missing something?

The advantages are that we preserve normal loading, and avoid any arbitrary signals to instead use duck-typing (i.e. merely requiring the method to exist in order to register it as a filter).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the goals is to only set up the register_block_type_args filter when the WP_Block_Supports class loaded in the runtime is the one from the plugin and not core, correct?

I would think this PR should also be ported to core for 5.6 as it is. Following that rationale my answer would be no.

Sure, there's the edge case of WP 5.6 beta 1, which already has a class different to this one. I'd argue that's a short-lived version for testing purposes, though. Anyway, the issue with inner blocks will be fixed anyway (because it's already fixed in core beta 1) and we don't need the current code in compat.php.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we go ahead and merge this, then? I've already ported the changes to WordPress/wordpress-develop#640 Would benefit from some orientation as to how to land that one.

@oandregal
Copy link
Member Author

I see a couple of e2e test that are failing, but I see (or can repro) the same failures on master. I'm going to go ahead and merge this one.

@oandregal oandregal merged commit cb3bcf2 into master Oct 22, 2020
@oandregal oandregal deleted the update/block-to-render-mechanism branch October 22, 2020 16:33
@github-actions github-actions bot added this to the Gutenberg 9.3 milestone Oct 22, 2020
@oandregal oandregal changed the title Update how block to render is tracked Block supports: refactor how the block to render is tracked to improve core/plugin compatibility Oct 22, 2020
@aristath
Copy link
Member

aristath commented Oct 23, 2020

I started getting PHP errors today, and after tracking things down with git bisect it turned out this PR was the culprit.

The PHP error I'm getting with an FSE theme now:

PHP Notice:  Undefined index: attrs in /var/www/html/wp-includes/class-wp-block-supports.php on line 86
PHP Stack trace:
PHP   1. {main}() /var/www/html/index.php:0
PHP   2. require() /var/www/html/index.php:17
PHP   3. require_once() /var/www/html/wp-blog-header.php:19
PHP   4. include() /var/www/html/wp-includes/template-loader.php:106
PHP   5. gutenberg_render_the_template() /var/www/html/wp-content/plugins/gutenberg/lib/template-canvas.php:19
PHP   6. do_blocks() /var/www/html/wp-content/plugins/gutenberg/lib/template-loader.php:367
PHP   7. render_block() /var/www/html/wp-includes/blocks.php:776
PHP   8. apply_filters() /var/www/html/wp-includes/blocks.php:686
PHP   9. WP_Hook->apply_filters() /var/www/html/wp-includes/plugin.php:212
PHP  10. gutenberg_render_block_with_assigned_block_context() /var/www/html/wp-includes/class-wp-hook.php:287
PHP  11. WP_Block->render() /var/www/html/wp-content/plugins/gutenberg/lib/compat.php:452
PHP  12. gutenberg_render_block_core_template_part() /var/www/html/wp-includes/class-wp-block.php:222
PHP  13. get_block_wrapper_attributes() /var/www/html/wp-content/plugins/gutenberg/build/block-library/blocks/template-part.php:66
PHP  14. WP_Block_Supports->apply_block_supports() /var/www/html/wp-includes/class-wp-block-supports.php:166

To replicate:

  • theme from w.org/themes/q
  • master branch of Gutenberg
  • WP 5.6-beta1-49280

cc @nosolosw @youknowriad


EDIT: Issue gets fixed after copying the class-wp-supports.php file from Gutenberg to core. So I suppose this is just a matter of back-porting the tweaks. However, it should be tested both with WP5.6 & 5.5 just to make sure there's no issues 👍

@oandregal
Copy link
Member Author

@aristath Thanks for reporting! I prepared WordPress/wordpress-develop#640 to fix this. Not sure how to move the related core patch forward, but it's ready for review/commit.

@talldan
Copy link
Contributor

talldan commented Oct 26, 2020

@nosolosw This PR seems to have introduced the following failing e2e test:

Meta boxes › Should render dynamic blocks when the meta box uses the excerpt for front end rendering

It looks like the Latest Posts block no longer renders with the .wp-block-latest-posts classname on its wrapping element.

@oandregal
Copy link
Member Author

Dan, there's WordPress/wordpress-develop#640 that will fix the issue. Riad is now merging into core so it can be part of Beta 2.

tellthemachines added a commit that referenced this pull request Oct 27, 2020
* Fix drop zone indicators for non blocks (#25986)

* Remove isDraggingBlocks check

* Clean up drag and drop data if the user presses escape when dragging

* Add comment to ensure avoidance of dragend

* Switch to using dragend

* Fix archives block render function (#26309)

* Code block: preserve indentation on paste (#26347)

* Turn off autocomplete for token input. (#26427)

* Fix parent post selector: ensure initial value available, search performed, all results shown. (#26397)

Co-authored-by: Nik Tsekouras <[email protected]>

* [RNMobile] Fix CI issues during `npm ci` (#26455)

* Run mobile tests on master

* Debug CI

* Revert "Debug CI"

This reverts commit dec1ebe.

* Update react-native-screens ref, pin @react-navigation/native version

* Pin @react-navigation/core,router versions

* Fix composer test failures due to invalid lock (#26472)

* Fix gallery block undo issue (#26377)

* Mark change in attributes when gallery mounts as not persistant

* Fix typo

* Fix package lock changes

* Fix some Twenty Twenty One related e2e test failures (#26341)

* Account for unknown number of controls in block inspector

* Ensure selection is on-screen by using a group block instead of cover, which takes up less space on screen

* Fix typewriter test

* Use correct name for patterns test

* Revert change to .wp-env.json

* Fix multi-entity side editor saving test

* Remove blank line

* insert paragraph instead of tempalte part (#26371)

* Fix autosave e2e tests (#26416)

* Update how block to render is tracked (#26356)

Co-authored-by: Daniel Richards <[email protected]>
Co-authored-by: Riad Benguella <[email protected]>
Co-authored-by: Ella van Durpe <[email protected]>
Co-authored-by: Adam Silverstein <[email protected]>
Co-authored-by: Nik Tsekouras <[email protected]>
Co-authored-by: Ceyhun Ozugur <[email protected]>
Co-authored-by: Marcus Kazmierczak <[email protected]>
Co-authored-by: Addison Stavlo <[email protected]>
Co-authored-by: O André <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants